Skip to content

fix: correct EnableAdcChannels docs from binary to decimal bitmask#160

Merged
tylerkron merged 3 commits intomainfrom
fix/enable-adc-channels-docs-binary-to-decimal
Apr 4, 2026
Merged

fix: correct EnableAdcChannels docs from binary to decimal bitmask#160
tylerkron merged 3 commits intomainfrom
fix/enable-adc-channels-docs-binary-to-decimal

Conversation

@tylerkron
Copy link
Copy Markdown
Contributor

Summary

  • The firmware's SCPI_ADCChanEnableSet parses the ENAble:VOLTage:DC parameter as a decimal integer via SCPI_ParamInt32, but the EnableAdcChannels XML docs, README, and DEVICE_INTERFACES.md all showed binary strings like "0000000011".
  • This mismatch meant callers following the documented examples would send the wrong channel configuration (e.g. firmware parses "11" as decimal 11 = channels 0,1,3 instead of intended channels 0,1).
  • Updated all docs and the test to use correct decimal bitmask values (e.g. "3" for channels 0+1, "84" for channels 2,4,6).

Related: daqifi/daqifi-desktop#439

Note: The daqifi-core-example-app also has this issue — its IsValidChannelMask() validates for binary strings and --channels help text likely shows binary format. That's a separate repo and should be fixed separately.

Test plan

  • All 818 core tests pass on both net8.0 and net9.0
  • Manual test: use example app with --channels 3 to enable channels 0+1

🤖 Generated with Claude Code

The firmware's SCPI_ADCChanEnableSet parses the ENAble:VOLTage:DC
parameter as a decimal integer via SCPI_ParamInt32, but the docs and
examples showed binary strings like "0000000011". This mismatch meant
any caller following the documented examples would send the wrong
channel configuration (e.g. "11" instead of "3" for channels 0+1).

Updated XML docs, README, and DEVICE_INTERFACES.md to show decimal
bitmask values. Updated the test to use a decimal example.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Entire-Checkpoint: a0fb8798807b
@tylerkron tylerkron requested a review from a team as a code owner March 29, 2026 15:54
@qodo-code-review
Copy link
Copy Markdown

ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan

Review Summary by Qodo

Fix EnableAdcChannels documentation from binary to decimal bitmask

🐞 Bug fix 📝 Documentation

Grey Divider

Walkthroughs

Description
• Corrects EnableAdcChannels parameter format from binary to decimal
• Updates XML docs with accurate decimal bitmask examples
• Fixes README and DEVICE_INTERFACES.md code examples
• Updates unit test to use correct decimal format
Diagram
flowchart LR
  A["Binary format<br/>0000000011"] -->|Incorrect| B["Firmware parses<br/>as decimal 11"]
  C["Decimal format<br/>3"] -->|Correct| D["Firmware parses<br/>as decimal 3"]
  B -->|Wrong channels| E["Channels 0,1,3"]
  D -->|Correct channels| F["Channels 0,1"]
  G["Updated docs<br/>& examples"] -->|References| C
Loading

Grey Divider

File Changes

1. src/Daqifi.Core.Tests/Communication/Producers/ScpiMessageProducerTests.cs 🧪 Tests +2/-2

Update test to use decimal bitmask format

• Changed EnableAdcChannels test from binary string "0001010100" to decimal "84"
• Updated expected command output to use decimal format

src/Daqifi.Core.Tests/Communication/Producers/ScpiMessageProducerTests.cs


2. src/Daqifi.Core/Communication/Producers/ScpiMessageProducer.cs 📝 Documentation +13/-14

Correct EnableAdcChannels XML docs to decimal format

• Updated XML documentation to clarify decimal bitmask format instead of binary
• Changed parameter description from binary string to decimal integer string
• Updated code examples to show decimal values (e.g., "84" for channels 2,4,6 and "3" for channels
 0,1)
• Added explanation of bit-to-channel mapping with decimal values

src/Daqifi.Core/Communication/Producers/ScpiMessageProducer.cs


3. src/Daqifi.Core/Device/DaqifiStreamingDevice.cs 📝 Documentation +2/-1

Update StartLoggingAsync docs to decimal bitmask

• Updated channelMask parameter documentation from binary to decimal format
• Changed example from "0000000011" to "3"
• Added clarification that firmware parses as decimal integer

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs


View more (3)
4. src/Daqifi.Core/Device/SdCard/ISdCardOperations.cs 📝 Documentation +2/-1

Update ISdCardOperations docs to decimal bitmask

• Updated channelMask parameter documentation from binary to decimal format
• Changed example from "0000000011" to "3"
• Added clarification that firmware parses as decimal integer

src/Daqifi.Core/Device/SdCard/ISdCardOperations.cs


5. README.md 📝 Documentation +1/-1

Update README example to decimal format

• Updated EnableAdcChannels example from binary "0000000011" to decimal "3"
• Added inline comment explaining bitmask calculation (0b11 = 3)

README.md


6. docs/DEVICE_INTERFACES.md 📝 Documentation +4/-4

Update DEVICE_INTERFACES.md examples to decimal format

• Updated three code examples to use decimal bitmask format
• Changed "0000000011" to "3" for channels 0 and 1
• Changed "11111111" to "255" for all 8 channels
• Updated comments to clarify decimal bitmask interpretation
• Added inline binary-to-decimal conversion explanations

docs/DEVICE_INTERFACES.md


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review bot commented Mar 29, 2026

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX Issues (0)

Grey Divider


Remediation recommended

1. Simulator docs still binary🐞 Bug ⚙ Maintainability
Description
After updating core docs to specify a decimal bitmask for ENAble:VOLTage:DC, the simulator
documentation still describes the parameter as a binary string and shows base-2 parsing. This leaves
contradictory guidance in-repo and can lead to an incorrect simulator implementation that won’t
match firmware behavior.
Code

src/Daqifi.Core/Communication/Producers/ScpiMessageProducer.cs[R318-336]

  /// <summary>
-    /// Creates a command message to enable ADC channels using a binary string.
+    /// Creates a command message to enable ADC channels using a decimal bitmask.
  /// </summary>
-    /// <param name="channelSetString">A binary string where each character represents a channel (0 = disabled, 1 = enabled), right-to-left. For example, "0001010100" enables channels 2, 4, and 6.</param>
+    /// <param name="channelSetString">A decimal integer string representing a bitmask where each bit enables a channel. For example, "84" (0b1010100) enables channels 2, 4, and 6.</param>
  /// <remarks>
-    /// The binary string is read from right to left, where position 0 is the rightmost bit:
-    /// - Position 0: Channel 0
-    /// - Position 1: Channel 1
-    /// - Position 2: Channel 2
+    /// The firmware parses this value as a decimal integer and interprets it as a bitmask:
+    /// - Bit 0 (value 1): Channel 0
+    /// - Bit 1 (value 2): Channel 1
+    /// - Bit 2 (value 4): Channel 2
  /// etc.
-    /// 
-    /// Command: ENAble:VOLTage:DC binaryString
-    /// Example: 
+    ///
+    /// Command: ENAble:VOLTage:DC decimalMask
  /// <code>
-    /// // Enable channels 2, 4, and 6
-    /// messageProducer.Send(ScpiMessageProducer.EnableAdcChannels("0001010100"));
-    /// 
-    /// // Enable channels 0 and 1
-    /// messageProducer.Send(ScpiMessageProducer.EnableAdcChannels("0000000011"));
+    /// // Enable channels 2, 4, and 6 (bitmask = 4 + 16 + 64 = 84)
+    /// device.Send(ScpiMessageProducer.EnableAdcChannels("84"));
+    ///
+    /// // Enable channels 0 and 1 (bitmask = 1 + 2 = 3)
+    /// device.Send(ScpiMessageProducer.EnableAdcChannels("3"));
  /// </code>
Evidence
Core docs now explicitly state the firmware parses the value as a decimal integer bitmask, but
simulator docs still specify `, show examples like 0000000011`, and even show
Convert.ToUInt32(binaryString, 2) parsing—contradicting the updated contract and recreating the
original mismatch in simulator guidance.

src/Daqifi.Core/Communication/Producers/ScpiMessageProducer.cs[318-341]
docs/simulator/SIMULATOR_DESIGN.md[139-160]
docs/simulator/SIMULATOR_DESIGN.md[404-416]
docs/simulator/SIMULATOR_IMPLEMENTATION_PLAN.md[583-590]
docs/simulator/GITHUB_ISSUES.md[258-266]
docs/simulator/GITHUB_ISSUES.md[280-287]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Simulator documentation still describes `ENAble:VOLTage:DC` as taking a binary string and shows base-2 parsing, which contradicts the updated core docs/tests that specify a decimal bitmask string.
### Issue Context
This PR corrects the public API documentation to match firmware behavior (decimal integer parsed as bitmask). Simulator docs should match the same contract to avoid future simulator/test implementations reintroducing the binary/decimal mismatch.
### Fix Focus Areas
- docs/simulator/SIMULATOR_DESIGN.md[139-160]
- docs/simulator/SIMULATOR_DESIGN.md[404-416]
- docs/simulator/SIMULATOR_IMPLEMENTATION_PLAN.md[583-590]
- docs/simulator/GITHUB_ISSUES.md[258-266]
- docs/simulator/GITHUB_ISSUES.md[280-287]
### What to change
- Replace `<binary_string>` with something like `<decimal_mask>` / `<decimal_bitmask>`.
- Update examples from `0000000011` to `3` (and similar).
- Update the simulator parsing example from `Convert.ToUInt32(binaryString, 2)` to decimal parsing (e.g., `Convert.ToUInt32(maskString)`), so it mirrors firmware behavior.
- Update the design doc’s example call `EnableAnalogChannels("0000000011")` to the correct API name and decimal value (e.g., `EnableAdcChannels("3")`), consistent with the rest of the repo.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

tylerkron and others added 2 commits April 3, 2026 22:08
Addresses Qodo review feedback — simulator docs still referenced binary
string format for ENAble:VOLTage:DC while core docs were updated to decimal.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@tylerkron tylerkron merged commit 3dfe93f into main Apr 4, 2026
1 check passed
@tylerkron tylerkron deleted the fix/enable-adc-channels-docs-binary-to-decimal branch April 4, 2026 04:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant